-
-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrote the PropertiesProvider API to support initialization, close and better multithreading #339
Conversation
1d7e972
to
ef7866d
Compare
Note: still need to be improved because it will slow down... |
ef7866d
to
4860641
Compare
4860641
to
03fe1a7
Compare
* @deprecated Use instead {@link #adjustProperties(AbstractLicenseMojo, Map, Document)} | ||
*/ | ||
@Deprecated | ||
default Map<String, String> getAdditionalProperties(AbstractLicenseMojo mojo, Properties currentProperties, Document document) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kept for backward compat' just in case...
03fe1a7
to
727497f
Compare
} | ||
|
||
@Override | ||
default void close() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addition
Map<String, String> getAdditionalProperties(AbstractLicenseMojo mojo, Properties currentProperties, Document document); | ||
public interface PropertiesProvider extends Closeable { | ||
|
||
default void init(AbstractLicenseMojo mojo, Map<String, String> currentProperties) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addition
...en-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightAuthorProvider.java
Show resolved
Hide resolved
...ven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightRangeProvider.java
Show resolved
Hide resolved
727497f
to
4b2cf41
Compare
license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitLookup.java
Show resolved
Hide resolved
// One-time warning for shallow repo | ||
if (mojo.warnIfShallow && !warnedIfShallow.get()) { | ||
SVNInfo info = svnClientManager.getWCClient().doInfo(documentFile, SVNRevision.HEAD); | ||
if (info.getDepth() != SVNDepth.INFINITY) { | ||
if (warnedIfShallow.compareAndSet(false, true)) { | ||
mojo.warn( | ||
"Sparse svn repository detected. Year and author property values may not be accurate."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbwiddis : FYI warnedIfShallow code updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first put this in I didn't realize the SVN provider didn't have the same set of properties. There are no authors to consider, and only the "last change" year is relevant (so a sparse repo may miss the latest commit of a file and default to current year). So this can probably just say "year" values like the adjusted git one above.
private final AtomicBoolean warnedIfShallow = new AtomicBoolean(); | ||
private final Queue<SVNClientManager> clients = new ConcurrentLinkedQueue<>(); | ||
private final ThreadLocal<SimpleDateFormat> sdfTimestampThreadLocal = ThreadLocal.withInitial( | ||
() -> new SimpleDateFormat("yyyyMMdd-HH:mm:ss")); | ||
private final ThreadLocal<SVNClientManager> svnClientThreadLocal = ThreadLocal.withInitial(() -> { | ||
SVNClientManager svnClientManager = svnCredentials == null ? | ||
SVNClientManager.newInstance(new DefaultSVNOptions()) : | ||
SVNClientManager.newInstance(new DefaultSVNOptions(), svnCredentials.getLogin(), | ||
svnCredentials.getPassword()); | ||
clients.offer(svnClientManager); | ||
return svnClientManager; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously incorrect resource cleanup: thread-local SVN clients were not closed. Kee ing track of all created clients per thread to clean them at the end.
Map<String, String> globalProperties = getDefaultProperties(); | ||
|
||
for (final Map.Entry<String, String> entry : mergeProperties(licenseSet, document).entrySet()) { | ||
if (entry.getValue() != null) { | ||
props.setProperty(entry.getKey(), entry.getValue()); | ||
} else { | ||
props.remove(entry.getKey()); | ||
} | ||
} | ||
for (final PropertiesProvider provider : propertiesProviders) { | ||
try { | ||
final Map<String, String> providerProperties = provider.getAdditionalProperties(AbstractLicenseMojo.this, props, document); | ||
if (getLog().isDebugEnabled()) { | ||
getLog().debug("provider: " + provider.getClass() + " brought new properties\n" + providerProperties); | ||
} | ||
for (Map.Entry<String, String> entry : providerProperties.entrySet()) { | ||
if (entry.getValue() != null) { | ||
props.setProperty(entry.getKey(), entry.getValue()); | ||
} else { | ||
props.remove(entry.getKey()); | ||
} | ||
} | ||
} catch (Exception e) { | ||
getLog().warn("failure occurred while calling " + provider.getClass(), e); | ||
} | ||
// we override by properties in the licenseSet | ||
if (licenseSet.properties != null) { | ||
for (Map.Entry<String, String> entry : licenseSet.properties.entrySet()) { | ||
if (!System.getProperties().contains(entry.getKey())) { | ||
globalProperties.put(entry.getKey(), entry.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid executing this code for each file.
|
||
for (final PropertiesProvider provider : ServiceLoader.load(PropertiesProvider.class, | ||
Thread.currentThread().getContextClassLoader())) { | ||
provider.init(this, globalProperties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init called here
@@ -685,6 +720,7 @@ public Properties load(final Document document) { | |||
|
|||
} finally { | |||
executorService.shutdownNow(); | |||
propertiesProviders.forEach(PropertiesProvider::close); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
close called here
4b2cf41
to
b33cef2
Compare
b33cef2
to
1c40385
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One suggested text change.
// One-time warning for shallow repo | ||
if (mojo.warnIfShallow && !warnedIfShallow.get()) { | ||
SVNInfo info = svnClientManager.getWCClient().doInfo(documentFile, SVNRevision.HEAD); | ||
if (info.getDepth() != SVNDepth.INFINITY) { | ||
if (warnedIfShallow.compareAndSet(false, true)) { | ||
mojo.warn( | ||
"Sparse svn repository detected. Year and author property values may not be accurate."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first put this in I didn't realize the SVN provider didn't have the same set of properties. There are no authors to consider, and only the "last change" year is relevant (so a sparse repo may miss the latest commit of a file and default to current year). So this can probably just say "year" values like the adjusted git one above.
…nd multithreading better
1c40385
to
1a936a5
Compare
synchronized
Ref: #335 | #327
CC: @philippn